tui: Fix TUI display issues in non-UTF-8 locales #1974

GabrielKimm · 2024-09-29T16:55:09Z

This commit addresses the issue #1870 where the TUI displays incorrect characters in non-UTF-8 locales. The fix includes:

Checking the current locale
Using ASCII characters for graph display in non-UTF-8 locales
Adjusting character widths accordingly

Fixes: #1870

Signed-off-by: Gabriel Kim [email protected]

GabrielKimm · 2024-09-29T17:05:27Z

I've addressed the issue #1870 regarding the TUI display in non-UTF-8 locales. Here's a summary of the changes and their effects:

Before the fix:

In non-UTF-8 locales, the TUI was displaying incorrect characters, making the graph structure unclear and difficult to read.

After the fix:

Now, the TUI correctly detects the locale setting and adjusts its display accordingly. In non-UTF-8 locales, it uses ASCII characters to represent the graph structure, ensuring clarity and readability across different environments.

Key changes:

Added locale detection to determine if UTF-8 is supported.
Implemented alternative ASCII characters for graph representation in non-UTF-8 locales.
Adjusted character width calculations to ensure proper alignment in both UTF-8 and non-UTF-8 environments.

These changes should resolve the display issues while maintaining the functionality and user experience of the TUI across various locale settings.

Please review and let me know if any further adjustments are needed.

cmds/tui.c

namhyung

Thanks for the update, only some nitpicks below.

cmds/tui.c

honggyukim · 2024-10-01T16:55:48Z

Hi @GabrielKimm, I see your name isn't correctly shown in the commit as follows.

commit 8340a73374fb17736ae81761d53acd6d351c5f5e (HEAD -> refs/heads/pull/1974)
Author:     0xGabriel <[email protected]>
AuthorDate: Mon Sep 30 01:51:49 2024 +0900

    Fix TUI display issues in non-UTF-8 locales
    
    This commit addresses the issue #1870 where the TUI displays
    incorrect characters in non-UTF-8 locales. The fix includes:
    
    - Checking the current locale
    - Using ASCII characters for graph display in non-UTF-8 locales
    - Adjusting character widths accordingly
    
    Fixes: #1870
    
    Signed-off-by: 0xGabriel <[email protected]>

Please put your real name rather than 0xGabriel.

You can check it at https://github.com/namhyung/uftrace/pull/1974.patch.

honggyukim · 2024-10-01T17:11:15Z

Your author name isn't changed yet. Please update it together.

From 39030abbd0de2289fa0600f0395b006176ff4c52 Mon Sep 17 00:00:00 2001
From: 0xGabriel <[email protected]>

As far as I remember, your name is "Gabriel Kim" so please use your full name.

You can update it as follows.

$ git commit --amend --author "Gabriel Kim <[email protected]>" --no-edit -s

honggyukim · 2024-10-01T17:12:39Z

In addition, you need to put category prefix for the commit title as follows.

tui: Fix TUI display issues in non-UTF-8 locales

It usually takes some time to get patches for review but it'd be useful to have a script for that. The 'checkout-pr.sh' script accepts a single parameter to get the pull request ID. It fetches from the source repo and checkouts to the fetched head. The example usage is as follows. $ ./misc/checkout-pr.sh 1974 % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 18502 100 18502 0 0 39899 0 --:--:-- --:--:-- --:--:-- 39875 remote: Enumerating objects: 1, done. remote: Counting objects: 100% (1/1), done. remote: Total 1 (delta 0), reused 1 (delta 0), pack-reused 0 (from 0) Unpacking objects: 100% (1/1), 354 bytes | 354.00 KiB/s, done. From https://github.com/GabrielKimm/uftrace * branch fix-tui-non-utf8-locale -> FETCH_HEAD Switched to and reset branch 'pull/1974' Now you're ready to review the pull request 1974 where can be found at namhyung#1974. Signed-off-by: Honggyu Kim <[email protected]>

honggyukim · 2024-10-01T17:40:32Z

Please use your name "Gabriel Kim" rather than "GabrielKim".

This commit addresses the issue namhyung#1870 where the TUI displays incorrect characters in non-UTF-8 locales. The fix includes: - Checking the current locale - Using ASCII characters for graph display in non-UTF-8 locales - Adjusting character widths accordingly Fixes: namhyung#1870 Signed-off-by: Gabriel Kim <[email protected]>

honggyukim · 2024-10-01T17:46:40Z

Thanks for the update. I will wait for @namhyung's review.

GabrielKimm · 2024-10-01T17:50:04Z

Thanks for the update. I will wait for @namhyung's review.

Thank you so much for your invaluable feedback and guidance on this PR.
Your detailed comments and suggestions have been incredibly helpful in improving the quality of my contribution.

I truly appreciate the time and effort you've put into reviewing my work and providing such constructive advice.

honggyukim

Thanks. I've just tested this and it looks fine but I have some concerns by checking the value of LANG.

The following can be another way to check if we can print graphical characters, but I'm not sure if it doesn't break other environments such as Android build, musl build, alpine Linux, etc.

#include <locale.h>
#include <wchar.h>

int can_print_utf8_graph_chars() {
    const char *graph_char = "★";
    wchar_t wc;

    setlocale(LC_CTYPE, "");

    int result = mbtowc(&wc, graph_char, MB_CUR_MAX);

    if (result > 0 && wc != L'\0')
        return true;

    return false;
}

cmds/tui.c

It usually takes some time to get patches for review but it'd be useful to have a script for that. The 'checkout-pr.sh' script accepts a single parameter to get the pull request ID. It fetches from the source repo and checkouts to the fetched head. The example usage is as follows. $ ./misc/checkout-pr.sh 1974 % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 18502 100 18502 0 0 39899 0 --:--:-- --:--:-- --:--:-- 39875 remote: Enumerating objects: 1, done. remote: Counting objects: 100% (1/1), done. remote: Total 1 (delta 0), reused 1 (delta 0), pack-reused 0 (from 0) Unpacking objects: 100% (1/1), 354 bytes | 354.00 KiB/s, done. From https://github.com/GabrielKimm/uftrace * branch fix-tui-non-utf8-locale -> FETCH_HEAD Switched to and reset branch 'pull/1974' Now you're ready to review the pull request 1974 where can be found at #1974. Signed-off-by: Honggyu Kim <[email protected]>

namhyung · 2024-10-06T05:44:38Z

It seems there are display issues still.

The cursor line overflows to the next line
alignment is broken with child functions - see set_program_name and strrchr.

GabrielKimm · 2024-10-06T08:38:57Z

It seems there are display issues still.

The cursor line overflows to the next line

alignment is broken with child functions - see set_program_name and strrchr.

Thank you for your thorough review and for pointing out these remaining display issues. I appreciate your attention to detail.

Cursor line overflow:
I understand the problem with the cursor line overflowing to the next line. This is likely due to an miscalculation in handling the terminal width or character width, especially in non-UTF-8 locales. I will review the code for string length calculations and line wrapping logic to address this issue.
Alignment of child functions:
Thank you for bringing this to my attention. The misalignment of child functions, particularly for set_program_name and strrchr, is indeed a critical issue for readability. I will carefully examine the indentation logic and how it handles various function names, especially longer ones. I'll also double-check the character width calculations for different locales to ensure consistent alignment.

I plan to make the following improvements:

Implement more robust terminal width detection and output adjustment.
Refine the indentation and alignment logic, particularly for child function display.
Conduct thorough testing across various locale settings to ensure consistency.
Review and optimize the string length and display width calculation logic.

I'll update the PR with these changes soon 😊🙏🏻🙏🏻

GabrielKimm force-pushed the fix-tui-non-utf8-locale branch from 96c38f3 to 33570dc Compare September 29, 2024 17:01

namhyung reviewed Sep 30, 2024

View reviewed changes

cmds/tui.c Outdated Show resolved Hide resolved

cmds/tui.c Outdated Show resolved Hide resolved

cmds/tui.c Outdated Show resolved Hide resolved

GabrielKimm force-pushed the fix-tui-non-utf8-locale branch from 33570dc to 8340a73 Compare September 30, 2024 06:47

namhyung reviewed Oct 1, 2024

View reviewed changes

cmds/tui.c Outdated Show resolved Hide resolved

cmds/tui.c Outdated Show resolved Hide resolved

GabrielKimm force-pushed the fix-tui-non-utf8-locale branch 2 times, most recently from cc92917 to 39030ab Compare October 1, 2024 17:06

GabrielKimm force-pushed the fix-tui-non-utf8-locale branch from 39030ab to 55a7afe Compare October 1, 2024 17:14

GabrielKimm force-pushed the fix-tui-non-utf8-locale branch 3 times, most recently from deb66a3 to 6f8a9c8 Compare October 1, 2024 17:19

GabrielKimm changed the title ~~Fix TUI display issues in non-UTF-8 locales~~ tui : Fix TUI display issues in non-UTF-8 locales Oct 1, 2024

GabrielKimm changed the title ~~tui : Fix TUI display issues in non-UTF-8 locales~~ tui: Fix TUI display issues in non-UTF-8 locales Oct 1, 2024

GabrielKimm requested a review from namhyung October 1, 2024 17:29

GabrielKimm force-pushed the fix-tui-non-utf8-locale branch from 6f8a9c8 to 72ef9b4 Compare October 1, 2024 17:41

GabrielKimm force-pushed the fix-tui-non-utf8-locale branch from 72ef9b4 to bfa6fce Compare October 1, 2024 17:42

honggyukim reviewed Oct 3, 2024

View reviewed changes

cmds/tui.c Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tui: Fix TUI display issues in non-UTF-8 locales #1974

tui: Fix TUI display issues in non-UTF-8 locales #1974

GabrielKimm commented Sep 29, 2024 •

edited

Loading

GabrielKimm commented Sep 29, 2024 •

edited

Loading

namhyung left a comment

honggyukim commented Oct 1, 2024

honggyukim commented Oct 1, 2024 •

edited

Loading

honggyukim commented Oct 1, 2024

honggyukim commented Oct 1, 2024

honggyukim commented Oct 1, 2024

GabrielKimm commented Oct 1, 2024

honggyukim left a comment •

edited

Loading

namhyung commented Oct 6, 2024

GabrielKimm commented Oct 6, 2024

tui: Fix TUI display issues in non-UTF-8 locales #1974

Are you sure you want to change the base?

tui: Fix TUI display issues in non-UTF-8 locales #1974

Conversation

GabrielKimm commented Sep 29, 2024 • edited Loading

GabrielKimm commented Sep 29, 2024 • edited Loading

namhyung left a comment

Choose a reason for hiding this comment

honggyukim commented Oct 1, 2024

honggyukim commented Oct 1, 2024 • edited Loading

honggyukim commented Oct 1, 2024

honggyukim commented Oct 1, 2024

honggyukim commented Oct 1, 2024

GabrielKimm commented Oct 1, 2024

honggyukim left a comment • edited Loading

Choose a reason for hiding this comment

namhyung commented Oct 6, 2024

GabrielKimm commented Oct 6, 2024

GabrielKimm commented Sep 29, 2024 •

edited

Loading

GabrielKimm commented Sep 29, 2024 •

edited

Loading

honggyukim commented Oct 1, 2024 •

edited

Loading

honggyukim left a comment •

edited

Loading